OCPEDGE-2094: Add console notification for pacemaker podman-etcd failures#1654
OCPEDGE-2094: Add console notification for pacemaker podman-etcd failures#1654lucaconsalvi wants to merge 10 commits into
Conversation
Add event recording to the TNF setup runner and etcd transition code so the full CEO-to-pacemaker handoff is observable via `kubectl get events -n openshift-etcd`. Nine events are emitted across the transition lifecycle: - runner.go: auth, cluster, fencing, etcd resource, constraints - etcd.go: transition started, waiting, static pod removed, completed Events use the direct K8s API (not library-go Recorder) since the setup runner is a short-lived Job. Failures are logged but never block the transition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The event reason (e.g. EtcdTransitionAuthCompleted) was used directly in metadata.name which should be a valid lowercase DNS subdomain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@lucaconsalvi: This pull request references OCPEDGE-2094 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR refactors pacemaker health evaluation, adds console health notifications, wires the new controller into startup, and records TNF setup transition events through Kubernetes Events. ChangesPacemaker and TNF observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant pacemakerInformer
participant consoleNotificationController
participant BuildHealthStatusFromCR
participant dynamicClient
pacemakerInformer->>consoleNotificationController: synchronize PacemakerCluster
consoleNotificationController->>BuildHealthStatusFromCR: build health status
BuildHealthStatusFromCR-->>consoleNotificationController: errors and warnings
consoleNotificationController->>dynamicClient: apply or delete console notification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/tnf/pkg/pacemaker/consolenotification_test.go (1)
38-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd tests for the ConsoleNotification lifecycle.
Health evaluation is covered, but creation, deletion, update drift, conflict/error handling, and preservation of unmanaged metadata are not. Exercise
syncwith a fake dynamic client before relying on this controller in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go` around lines 38 - 196, Add lifecycle-focused tests for the ConsoleNotification controller’s sync flow, using a fake dynamic client to cover creation, deletion, update drift, resource conflicts/errors, and preservation of unmanaged metadata. Keep the existing evaluateHealth tests unchanged, and assert the resulting resource state and propagated handling through the relevant sync/controller methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go`:
- Around line 104-120: Update the “offline node skips resource and fencing
checks” test setup around makeNodeWithConditions to explicitly set the node’s
Etcd Healthy condition to False before evaluation, while preserving the offline
and fencing-unavailable conditions and the expected offline-only notification.
In `@pkg/tnf/pkg/pacemaker/consolenotification.go`:
- Line 180: Update the logging statements in the console notification creation
and related paths around klog.Infof so they do not include the notification
text; instead log the operation and notification problem count, avoiding
exposure of node hostnames while retaining useful summary information.
- Around line 102-112: Update the Pacemaker status checks around
getConditionStatus and the cr.Status.Nodes guard so uninitialized status is
reported as unhealthy: flag zero LastUpdated, treat missing or
metav1.ConditionUnknown conditions as problems, and report nil or empty Nodes
according to the CRD’s catastrophic-state guidance. Preserve existing stale,
maintenance, and healthy-status behavior, and add coverage for uninitialized and
Unknown statuses.
- Around line 187-195: Update the reconciliation logic around existingText and
the ConsoleNotification Update call to compare and reconcile every
controller-owned field, including backgroundColor, color, link, location, and
text. Mutate a copy of existing rather than updating u directly, preserve its
annotations, finalizers, labels, and other metadata, set the desired owned
fields on the copy, retain the existing resource version, and submit that copy
through the dynamic client.
---
Nitpick comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go`:
- Around line 38-196: Add lifecycle-focused tests for the ConsoleNotification
controller’s sync flow, using a fake dynamic client to cover creation, deletion,
update drift, resource conflicts/errors, and preservation of unmanaged metadata.
Keep the existing evaluateHealth tests unchanged, and assert the resulting
resource state and propagated handling through the relevant sync/controller
methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d68afcfd-0760-4e1e-83e7-699379c44f13
📒 Files selected for processing (3)
pkg/tnf/operator/starter.gopkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.go
Manual Testing on TNF ClusterTested the console notification on a live TNF baremetal cluster deployed via cluster-bot with a custom CEO image built from this PR. Cluster: Test 1 — Banner appears on pacemaker failureTriggered maintenance mode via Red banner visible at the top of the OpenShift web console with the "Troubleshooting guide" link. Test 2 — Banner disappears when health is restoredDisabled maintenance mode via PacemakerCluster CR returned to ResultPASS — Full lifecycle verified: banner creation, content accuracy, troubleshooting link, and automatic cleanup on health restoration. 
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Review: requesting changes
Four items, one blocking.
1. [Blocking] evaluateHealth re-derives health logic that healthcheck.go already computes — and the two implementations already disagree
I think we can get a much leaner code by trying to reuse the existing logic instead of reimplementing it outselves
evaluateHealth() checks staleness, maintenance mode, offline nodes, unhealthy etcd, and fencing unavailability — the same conditions healthcheck.go's buildHealthStatusFromCR / checkClusterConditions / checkNodeStatuses already evaluate. Two independent implementations in the same package is a drift magnet, and the drift has already happened:
-
Zero
LastUpdated:healthcheck.go:292treatsLastUpdated.IsZero()as an error ("CR has no status populated").evaluateHealth:102guards the staleness check with!cr.Status.LastUpdated.IsZero(), so a status collector that never successfully runs (crash-loop, RBAC, image pull failure) produces zero-value LastUpdated forever — and the banner never appears. This is the exact scenario the banner exists to surface. -
Kubelet health silently skipped:
evaluateHealth:125hasif res.Name != PacemakerClusterResourceNameEtcd { continue }, filtering out Kubelet entirely.healthcheck.go:563(getNodeResourceSummaries) iterates all resources unfiltered — confirmed byhealthcheck_test.go'sunhealthy_kubelettest case. Result:oc get co etcdshows Degraded=True from Kubelet failure, but the console banner stays silent.
The PR title scopes to "pacemaker podman-etcd failures" but evaluateHealth also checks staleness, maintenance mode, offline nodes, and fencing — it is general health, not etcd-specific. The narrow Jira (OCPEDGE-2094) contributed to the etcd-only resource filter, but we should consolidate all notification conditions here rather than building incrementally on the duplication. Let's use this PR to add all the notifications we think we want, no need to limit ourselves by the scope defined 1 year ago.
Proposed fix: Have the console notification controller consume the HealthStatus that healthcheck.go already produces (or extract a shared health evaluation function). The controller's job is presentation — translating problems into banner text — not re-deriving what healthcheck already knows. The HealthStatus.Errors and HealthStatus.Warnings slices already contain the human-readable strings this controller needs.
2. No Console capability check — infinite error loop on edge clusters
If the Console cluster capability is disabled (common on minimal/edge footprints — TNF's target), the consolenotifications API does not exist. Every 30s sync fails with an API error, logged and retried forever. No degraded condition, no circuit breaker — permanent log noise for the lifetime of the cluster.
The codebase has an established pattern for this: ceohelpers/machine_api.go checks ClusterVersionCapabilityMachineAPI before touching machine API resources. Follow the same pattern — check Console capability at startup or on first sync failure, log once at Info, and no-op all subsequent syncs.
3. Hand-rolled CRUD — use library-go resourceapply helpers
ensureNotification / deleteNotification hand-roll ~70 lines of Get→Create/Update and Delete logic. vendor/.../resourceapply/monitoring.go exports ApplyUnstructuredResourceImproved and DeleteUnstructuredResource — both already used in this codebase for ServiceMonitor, PrometheusRule, and Alertmanager resources. They handle metadata merge, event recording, and NotFound tolerance out of the box.
The hand-rolled version also skips event recording entirely — eventRecorder is available (passed to the factory) but never used for ConsoleNotification CRUD. Every other controller in this codebase that creates/updates resources emits recorder events. Switching to the library-go helpers fixes this for free and drops ~50 lines.
4. Add sync-level tests after the refactor
Only evaluateHealth is tested today. Once the refactor from point 1 and the resourceapply switch from point 3 land, the remaining testable surface is the sync() orchestration: "given this HealthStatus, do we create, update, or delete the banner?" and the banner text formatting. The codebase already has fakedynamic.NewSimpleDynamicClient in starter_test.go — use it to cover the sync paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tnf/pkg/pacemaker/consolenotification.go (1)
180-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
consoleUnavailableis latched permanently and never re-checked.Once
meta.IsNoMatchErroris hit,consoleUnavailableis set totrueandsyncreturns early forever. On clusters where the Console API/CRD is installed after this operator starts (or re-registered), notification management stays disabled until the operator restarts. Consider periodically re-probing (e.g. reset the flag on resync) rather than latching it for the process lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tnf/pkg/pacemaker/consolenotification.go` around lines 180 - 190, The consoleUnavailable flag set by filterConsoleError must not permanently disable notification management after a transient NoMatchError. Update the sync/resync flow and filterConsoleError so the flag is periodically reset and the Console API is re-probed, while preserving the existing behavior of ignoring NoMatchError and returning other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification.go`:
- Around line 128-134: Update the notification flow around
BuildHealthStatusFromCR and classifyProblems to detect a PacemakerCluster CR
with zero LastUpdated before classifying problems, then remove any existing
console notification and return without creating a banner. Add a sync test
covering this uninitialized-status case.
---
Nitpick comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification.go`:
- Around line 180-190: The consoleUnavailable flag set by filterConsoleError
must not permanently disable notification management after a transient
NoMatchError. Update the sync/resync flow and filterConsoleError so the flag is
periodically reset and the Console API is re-probed, while preserving the
existing behavior of ignoring NoMatchError and returning other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c80ba177-91d4-4170-8942-cf31caf045f7
📒 Files selected for processing (9)
hack/test-transition-events.shpkg/tnf/pkg/etcd/etcd.gopkg/tnf/pkg/etcd/etcd_test.gopkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.gopkg/tnf/pkg/pacemaker/healthcheck.gopkg/tnf/pkg/pacemaker/healthcheck_test.gopkg/tnf/pkg/tools/events.gopkg/tnf/setup/runner.go
TNF-specific manual validation script belongs under the TNF directory tree rather than the repo-wide hack/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tnf-test=true label to mock events so the cleanup command only targets test events, not real cluster diagnostic data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ures Surface pacemaker health problems directly in the OpenShift web console via a ConsoleNotification banner. When etcd resources fail, fencing becomes unavailable, or pacemaker status goes stale, a red banner appears linking to the troubleshooting guide. The banner is removed automatically when health is restored. New controller shares the existing PacemakerCluster informer with the healthcheck and metrics controllers, following the same registration pattern in runPacemakerControllers(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Improves readability of multi-problem notifications by joining problems with ". " instead of "; ". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The "offline node skips resource and fencing checks" test claimed etcd was unhealthy but makeNodeWithConditions creates healthy resources. Add withUnhealthyResource helper and use it so the test genuinely verifies that offline nodes skip resource checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Separate pacemaker health problems into two notification categories (degraded-operations vs troubleshooting) so each banner links to the relevant documentation section. Consolidate shared logic, add test helpers, and point doc URLs to the published Red Hat docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the CR exists but the status collector CronJob hasn't run yet (LastUpdated is zero), treat it the same as a missing CR to avoid a false-positive troubleshooting banner at startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f6b061e to
b5e1f48
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/hack/test-transition-events.sh`:
- Around line 65-66: Add an EXIT trap in test-transition-events.sh that executes
oc delete events for the tnf-test=true label in ${NAMESPACE}, ensuring cleanup
runs on both success and failure. Replace the existing cleanup-only echo with
the actual trap while preserving the current namespace and label selection.
- Around line 26-54: Update the test flow around the event-creation loop and its
callers so events are emitted through the production TNF transition path,
including RecordSetupEvent, rather than being created directly by this script.
Remove the manually fabricated event fixtures, then validate the actual emitted
events and messages, especially EtcdTransitionCompleted, against the production
behavior in the TNF transition flow.
- Line 62: Update the event validation command in test-transition-events.sh to
use a valid field selector and preserve failures from both oc and grep instead
of converting them to success. Validate every expected EtcdTransition event’s
reason, type, source, and message, while treating a genuine absence of matching
events as a failure rather than merely printing a fallback message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e0e8907e-ffc3-4a82-bf51-d375595340f3
📒 Files selected for processing (10)
pkg/tnf/hack/test-transition-events.shpkg/tnf/operator/starter.gopkg/tnf/pkg/etcd/etcd.gopkg/tnf/pkg/etcd/etcd_test.gopkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.gopkg/tnf/pkg/pacemaker/healthcheck.gopkg/tnf/pkg/pacemaker/healthcheck_test.gopkg/tnf/pkg/tools/events.gopkg/tnf/setup/runner.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/tnf/pkg/etcd/etcd.go
- pkg/tnf/setup/runner.go
- pkg/tnf/pkg/pacemaker/consolenotification.go
- pkg/tnf/pkg/etcd/etcd_test.go
- pkg/tnf/pkg/tools/events.go
- pkg/tnf/operator/starter.go
- pkg/tnf/pkg/pacemaker/consolenotification_test.go
- pkg/tnf/pkg/pacemaker/healthcheck.go
- pkg/tnf/pkg/pacemaker/healthcheck_test.go
|
/approve |
|
/lgtm |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test e2e-agnostic-ovn |
|
@lucaconsalvi: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
ConsoleNotificationControllerthat watches thePacemakerClusterCR and manages aConsoleNotificationbanner in the OpenShift web console when pacemaker health problems are detectedPacemakerClusterinformer with healthcheck and metrics controllers, registered alongside them inrunPacemakerControllers()Files Changed
pkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.gopkg/tnf/operator/starter.gorunPacemakerControllers()Test plan
Jira: https://issues.redhat.com/browse/OCPEDGE-2094
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests